-
Notifications
You must be signed in to change notification settings - Fork 187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Verdi: Load config / profile lazily to speed up tab-completion #6140
Conversation
@sphuber can you take a quick look to see if I am on the right track here? (no rush) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far nobody replied on the click repo so we'll have to deal with autocompletion ourselved. But I'll do that in a separate PR so that we can focus here on lazy config loading. So this is ready for review @sphuber
profile_uuid = str(uuid.uuid4) | ||
options = [ | ||
'--non-interactive', '--profile', profile_name, '--profile-uuid', profile_uuid, '--db-backend', | ||
self.storage_backend_name, '--db-name', db_name, '--db-username', db_user, '--db-password', db_pass, | ||
'--db-port', self.pg_test.dsn['port'] | ||
'--db-port', self.pg_test.dsn['port'], '--email', user_email |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test started failing with the changes introduced here. I am unsure if that means that I broke something, or whether the test worked by accident.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually looks like a regression because you made the default
for this option lazy with a functools partial. I don't think the partial is properly called, so the value that is passed for the option is the partial and not the actual config option value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, after going through the guts of click and its modification in aiida, I've determined that the default value seems to be evaluating correctly, i.e. the callback is called, but it returns an empty tuple. I think the test worked previously because the default was evaluated before the actual test, and hence the get_config('autofill.user.email')
was called in the local context, not in test context, where this value seems to be unavailable.
@sphuber could you maybe try if you can make the test fail locally on main branch in a fresh aiida install without any profile? That would I think confirm my hypothesis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. The problem is that the cmd_setup
module is imported at the top of the test file. This will evaluate the setup
command and with it the defaults. So even if I add the empty_config
fixture to the test_setup_profile_uuid
test, by the time that fixture gets invoked to provide an empty config for the test, the defaults of the setup command will already have been set. So by making the defaults lazily evaluated, this would indeed explain the observed behavior.
The `VerdiContext` class, which provides the custom context of the `verdi` commands, loads the configuration. This has a non-negligible cost and so slows down the responsiveness of the CLI. This is especially noticeable during tab-completion. The `obj` custom object of the `VerdiContext` is replaced with a subclass of `AttributeDict` that lazily populates the `config` key when it is called with the loaded `Config` class. In addition, the defaults of some options of the `verdi setup` command, which load a value from the config and so require the config, are turned into partials such that they also are lazily evaluated. These changes should give a reduction in load time of `verdi` of the order of ~50 ms. A test of `verdi setup` had to be updated to explicitly provide a value for the email. This is because now the default is evaluated lazily, i.e. when the command is actually called in the test. At this point, there is no value for this config option and so the default is empty. Before, the default would be evaluated as soon as `aiida.cmdline.commands.cmd_setup` was imported, at which point an existing config would still contain these values, binding them to the default, even if the config would be reset afterwards before the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @danielhollas
As discussed in #6117 (comment), we want to avoid loading the config file in verdi CLI until it is really needed. This is especially important for speeding up tab-completion.
There are several changes here:
VerdiContext
has only been introduced recently in CLI: allow setting options for config without profiles #5544, previously the config was loaded lazily as well it seems.ctx.resilient_parsing
. EDIT: Postponed for a separate PR.For point 3 I created an issue on the click repo, let's see what they say pallets/click#2614
Benchmark after implementing points 1. and 2.
verdi --version
(main)verdi --version
(this PR)